Update SwappableLock to support NET9+ Lock type#1077
Update SwappableLock to support NET9+ Lock type#1077dwcullop wants to merge 1 commit intoreactivemarbles:mainfrom
Conversation
Add Lock overloads for SwappableLock.SwapTo and constructor to support the new System.Threading.Lock type on .NET 9+. Uses #if NET9_0_OR_GREATER conditional compilation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dwcullop
changed the title
There was a problem hiding this comment.
Pull request overview
Adds .NET 9+ System.Threading.Lock support to SwappableLock so ObservableCache can atomically swap between synchronization primitives without forcing object/Monitor locks on newer runtimes.
Changes:
- Added
CreateAndEnter(Lock gate)factory for NET9+. - Added
SwapTo(Lock gate)overload and updatedSwapTo(object gate)to correctly release whichever gate type is currently held. - Updated
Dispose()to release a heldLockon NET9+.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #if NET9_0_OR_GREATER | ||
| public static SwappableLock CreateAndEnter(Lock gate) | ||
| { |
There was a problem hiding this comment.
On NET9+, the Lock overloads can still receive null at runtime (despite non-nullable annotations). Currently that would throw a NullReferenceException from gate.Enter()/Exit(), unlike the object overloads which throw ArgumentNullException. Consider adding an explicit null guard (e.g., ArgumentNullException.ThrowIfNull(gate)) in CreateAndEnter(Lock) to provide a consistent and clearer failure mode.
| { | |
| { | |
| System.ArgumentNullException.ThrowIfNull(gate); |
| public void SwapTo(Lock gate) | ||
| { | ||
| if (_lockGate is null && _gate is null) | ||
| throw new InvalidOperationException("Lock is not initialized"); | ||
|
|
||
| gate.Enter(); | ||
|
|
There was a problem hiding this comment.
SwapTo(Lock gate) should guard against a null Lock argument as well; otherwise gate.Enter() will throw NullReferenceException. Adding an explicit ArgumentNullException for a null gate keeps behavior consistent with Monitor.Enter on the object overloads and produces a more actionable exception.
| #if NET9_0_OR_GREATER | ||
| public static SwappableLock CreateAndEnter(Lock gate) | ||
| { | ||
| gate.Enter(); | ||
| return new SwappableLock() { _lockGate = gate }; | ||
| } | ||
| #endif | ||
|
|
||
| public void SwapTo(object gate) | ||
| { | ||
| #if NET9_0_OR_GREATER | ||
| if (_gate is null && _lockGate is null) | ||
| throw new InvalidOperationException("Lock is not initialized"); | ||
| #else | ||
| if (_gate is null) | ||
| throw new InvalidOperationException("Lock is not initialized"); | ||
| #endif | ||
|
|
||
| var hasNewLock = false; | ||
| Monitor.Enter(gate, ref hasNewLock); | ||
|
|
||
| #if NET9_0_OR_GREATER | ||
| if (_lockGate is not null) | ||
| { | ||
| _lockGate.Exit(); | ||
| _lockGate = null; | ||
| } | ||
| else | ||
| #endif | ||
| if (_hasLock) | ||
| Monitor.Exit(_gate); | ||
| { | ||
| Monitor.Exit(_gate!); | ||
| } | ||
|
|
||
| _hasLock = hasNewLock; | ||
| _gate = gate; | ||
| } | ||
|
|
||
| #if NET9_0_OR_GREATER | ||
| public void SwapTo(Lock gate) | ||
| { | ||
| if (_lockGate is null && _gate is null) | ||
| throw new InvalidOperationException("Lock is not initialized"); | ||
|
|
||
| gate.Enter(); | ||
|
|
||
| if (_lockGate is not null) | ||
| _lockGate.Exit(); | ||
| else if (_hasLock) | ||
| Monitor.Exit(_gate!); | ||
|
|
||
| _lockGate = gate; | ||
| _hasLock = false; | ||
| _gate = null; | ||
| } |
There was a problem hiding this comment.
This PR adds new NET9+ behavior (CreateAndEnter(Lock), SwapTo(Lock), and mixed-type swapping/release logic) but there are no tests covering it. Since the test project targets net9.0, it should be feasible to add unit tests that validate swapping object→Lock, Lock→object, and Dispose releasing the correct gate.
| private bool _hasLock; | ||
| private object? _gate; | ||
|
|
||
| #if NET9_0_OR_GREATER |
There was a problem hiding this comment.
I think we really need to split the whole class for .NET 9 versus earlier versions.
It's weird enough to have both an object? gate and a Lock? gate within the same instance, when you'd never actually want to use both, but then it's even weirder that the API surface of SwappableLock allows it. So, you could end up accidentally having the same SwappableLock instance track both an object-based and a Lock-based lock at the same time, and get some really weird behavior.
Either we should just add a whole new SwappableThreadingLock class for System.Threading.Lock or actually DISABLE the object-based mechanics when System.Threading.Lock is available, and force all consumers to swap to using System.Threading.Lock for .NET 9+.
Since this is an internal utility, I kinda like option 2. Just only use Lock-based locks.
Description:
SwappableLock is a ref struct used internally by ObservableCache to atomically swap between lock objects during write operations. It currently only supports Monitor.Enter/Monitor.Exit on object gates.
.NET 9 introduced System.Threading.Lock: a dedicated, more efficient lock type that doesn't support Monitor.Enter. This PR adds overloads so SwappableLock works with both object (pre-.NET 9) and Lock (.NET 9+).
Changes
Uses #if NET9_0_OR_GREATER conditional compilation. Zero behavioral change on pre-.NET 9 targets.
Why this matters
ObservableCache needs to swap locks atomically during write operations (e.g., from the notification lock to the writer lock). Without Lock support, .NET 9+ code paths that use Lock elsewhere would be incompatible with SwappableLock, forcing fallback to object locks and losing the performance benefits of the new Lock type.